[CALCITE-7620] Result of FILTER clause in window functions is incorrect#5040
Conversation
| * partition by ORDER BY keys. The output order is therefore not defined by | ||
| * a simple collation in the general case, so we conservatively report no | ||
| * collations. */ | ||
| public static @Nullable List<RelCollation> window(RelMetadataQuery mq, RelNode input, |
There was a problem hiding this comment.
The reason for modifying RelMdCollation.window is that the original window sorting derivation was too optimistic, which would cause the optimizer to mistakenly believe that the window output retained the input order, thus mistakenly deleting the top-level Sort.
The original implementation had the following problem:
-
Previously,
RelMdCollation.windowdirectly returnedmq.collations(input), meaning "the window operator will preserve the order of the input rows as is." However, the actual implementation ofEnumerableWindowfirst groups the rows by thePARTITION BYkey usingSortedMultiMap, and then sorts them within each group by thewindow ORDER BYkey. Therefore, the input order is not preserved; the global output order isPARTITION BY keys + ORDER BY keys, not simply the input order.
This caused the top-levelSortto be incorrectly optimized away. -
When
order by empnois written in the SQL, if the window also happens to be sorted byempno, the optimizer will mistakenly assume that the window output is globally ordered, thus deleting the top-levelEnumerableSort. The resulting output is grouped bydeptno, not sorted byempno.
| this.hints = calc.getHints(); | ||
| this.cluster = calc.getCluster(); | ||
| this.traits = calc.getTraitSet(); | ||
| this.traits = calc.getTraitSet() |
There was a problem hiding this comment.
The reason for modifying CalcRelSplitter.java is that when ProjectToWindowRule splits Calc/Project containing window functions, it passes the original node's trait set (including the contaminated collation) to the new node after splitting, causing the optimizer to incorrectly remove the top-level Sort before the window expands.
| !ok | ||
|
|
||
| # Test 3: Multiple FILTER with OVER on different aggregates | ||
| select empno, deptno, |
There was a problem hiding this comment.
another case
select ename, job, hiredate,
avg(sal) over (order by hiredate rows 3 preceding) as avg_sal,
avg(sal) filter (where job = 'MANAGER') over (order by hiredate rows 3 preceding)
as avg_mgr_sal
from emp
order by hiredate;There was a problem hiding this comment.
OK, this test has been added; the AVG_MGR_SAL field is related to this filter modification, and the data is consistent with https://onecompiler.com/postgresql/44smkpfxb
|
Do you happy with current changes? Because this issue affects the data quality of the filter, want to fix it as soon as possible so that this capability is usable in production. PTAL~ @iwanttobepowerful |
| EnumerableWindow(window#0=[window(partition {0} rows between UNBOUNDED PRECEDING and CURRENT ROW aggs [ROW_NUMBER()])], constants=[[false]]) | ||
| EnumerableCalc(expr#0..2=[{inputs}], DEPTNO=[$t0]) | ||
| EnumerableTableScan(table=[[scott, DEPT]]) | ||
| EnumerableCalc(expr#0..2=[{inputs}], expr#3=[1], expr#4=[<=($t2, $t3)], proj#0..2=[{exprs}], $condition=[$t4]) |
There was a problem hiding this comment.
Curious about the motivation behind this update?
There was a problem hiding this comment.
This fix altered the sorting metadata derivation for the window operator, resulting in a legitimate change to the execution plan structure of two existing !plan tests, but the computation results remain unchanged.
Specific changes: Previously, the collation derivation for EnumerableWindow was optimistic, claiming to retain the input sorting. With CalcRelSplitter inheriting the original Calc's collation, the optimizer would push Sort down onto the Window and merge it with the outer Calc. Therefore, the original plan was:
EnumerableSort
EnumerableCalc(condition + window output)
EnumerableWindow
After the fix, EnumerableWindow no longer claims any sorting, and CalcRelSplitter no longer inherits the contaminated collation. Sort can only stop at the Window, wrapped in an outer Calc for projection. The plan becomes:
EnumerableCalc(projection)
EnumerableSort
EnumerableCalc(condition)
EnumerableWindow
Both are logically equivalent; only the relative positions of Sort and the projected Calc have changed. Therefore, only the expected output of these two !plan blocks was updated, without modifying any result data.This also ensures complete consistency with the PostgreSQL sorting results.
There was a problem hiding this comment.
The plan generated by the new correlation algorithm looks much better.
!use scott
!set outputformat mysql
select sal from emp e
where 123 NOT IN (
select cast(null as int)
from dept d where e.deptno=d.deptno);
+-----+
| SAL |
+-----+
+-----+
(0 rows)
!ok
EnumerableCalc(expr#0..3=[{inputs}], expr#4=[NOT($t3)], SAL=[$t1], $condition=[$t4])
EnumerableHashJoin(condition=[AND(IS NOT DISTINCT FROM($2, $4), =(123, $3))], joinType=[left_mark])
EnumerableCalc(expr#0..7=[{inputs}], EMPNO=[$t0], SAL=[$t5], DEPTNO=[$t7])
EnumerableTableScan(table=[[scott, EMP]])
EnumerableCalc(expr#0..2=[{inputs}], expr#3=[null:INTEGER], $f0=[$t3], DEPTNO=[$t0])
EnumerableTableScan(table=[[scott, DEPT]])
!plan
There was a problem hiding this comment.
The plan generated by the new correlation algorithm looks much better.
!use scott !set outputformat mysql select sal from emp e where 123 NOT IN ( select cast(null as int) from dept d where e.deptno=d.deptno); +-----+ | SAL | +-----+ +-----+ (0 rows) !ok EnumerableCalc(expr#0..3=[{inputs}], expr#4=[NOT($t3)], SAL=[$t1], $condition=[$t4]) EnumerableHashJoin(condition=[AND(IS NOT DISTINCT FROM($2, $4), =(123, $3))], joinType=[left_mark]) EnumerableCalc(expr#0..7=[{inputs}], EMPNO=[$t0], SAL=[$t5], DEPTNO=[$t7]) EnumerableTableScan(table=[[scott, EMP]]) EnumerableCalc(expr#0..2=[{inputs}], expr#3=[null:INTEGER], $f0=[$t3], DEPTNO=[$t0]) EnumerableTableScan(table=[[scott, DEPT]]) !plan
Yes, this explain is more fitable !
| this.cluster = calc.getCluster(); | ||
| this.traits = calc.getTraitSet(); | ||
| this.traits = calc.getTraitSet() | ||
| .replaceIfs(RelCollationTraitDef.INSTANCE, Collections::emptyList); |
There was a problem hiding this comment.
The collation reset should be scoped to the LogicalWindow creation rather than applied in CalcRelSplitter’s constructor. The ordering issue is specific to window nodes: a Window does not preserve or expose a simple output collation, so it should not inherit the original Calc collation trait.
Suggested change:
ProjectToWindowRule.java:260
@Override protected RelNode makeRel(RelOptCluster cluster, RelTraitSet traitSet,
RelBuilder relBuilder, RelNode input, RexProgram program, List<RelHint> hints) {
checkArgument(program.getCondition() == null,
"WindowedAggregateRel cannot accept a condition");
+ traitSet = traitSet.replaceIfs(RelCollationTraitDef.INSTANCE,
+ Collections::emptyList);
return LogicalWindow.create(cluster, traitSet, relBuilder, input,
program, hints);
}
This keeps the generic splitter behavior unchanged and makes the intent local to the window-producing RelType.
| SqlCall call = (SqlCall) node; | ||
| bb.getValidator().deriveType(bb.scope, call); | ||
| SqlCall aggCall = call.operand(0); | ||
| @Nullable SqlNode filter = null; |
There was a problem hiding this comment.
This is to satisfy the checkframe null-safety check; otherwise, the calling method would need to be modified, whereas here, simply adding an annotation resolves the issue.
| Util.discard(mq); | ||
| Util.discard(input); | ||
| Util.discard(groups); | ||
| return Collections.emptyList(); |
There was a problem hiding this comment.
| Util.discard(mq); | |
| Util.discard(input); | |
| Util.discard(groups); | |
| return Collections.emptyList(); | |
| return Collections.emptyList(); |
There was a problem hiding this comment.
Done, before this just to handle with unused warning.
| * Applies a FILTER clause to the arguments of an aggregate call by wrapping | ||
| * each argument in a CASE expression. For example, | ||
| * {@code SUM(sal) FILTER (WHERE comm IS NOT NULL)} becomes | ||
| * {@code SUM(CASE WHEN comm IS NOT NULL THEN sal END)}. |
There was a problem hiding this comment.
I'm unsure if the semantics of functions like FIRST_VALUE, LAST_VALUE, NTH_VALUE, LEAD, and LAG are correct after the rewrite.
There was a problem hiding this comment.
Your concern is valid. The rewrite is only semantically correct for true aggregate functions like SUM, COUNT, AVG, MIN, and MAX, because those functions ignore NULL inputs.
However filter does not hold for window value functions such as FIRST_VALUE, LAST_VALUE, NTH_VALUE, LEAD, and LAG. such as sql in postgresql would error out:
SELECT
ename,
job,
FIRST_VALUE(sal) FILTER (WHERE job = 'MANAGER') OVER (ORDER BY hiredate, ename) AS first_mgr_sal
FROM emp
ORDER BY hiredate, ename;
psql:commands.sql:81: ERROR: FILTER is not implemented for non-aggregate window functions
LINE 4: FIRST_VALUE(sal) FILTER (WHERE job = 'MANAGER') OVER (ORDE...
Therefore, based on the tests conducted so far, I believe there are no issues.
There was a problem hiding this comment.
Could you add a related test?
|
|
If there no other comments, I would merge this pr soon. cc @iwanttobepowerful |
|
Sorry, I’m not too familiar with this area. |
Alright, the current issues should all be addressed. Let's see if there are any other suggestions; if not, I'll merge this in 48 hours. This is a fairly urgent matter—fixing data quality issues. |



jira: https://issues.apache.org/jira/browse/CALCITE-7620